-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
main: help subcommand should work without root. #31
Conversation
Could also try to use another function that takes the subcommand directly, which probably exists. |
cmd/sbctl/main.go
Outdated
if strings.Contains(c.CommandPath(), "completion zsh") || | ||
strings.Contains(c.CommandPath(), "completion bash") || | ||
strings.Contains(c.CommandPath(), "completion fish") || | ||
strings.Contains(c.CommandPath(), "__complete") { | ||
strings.Contains(c.CommandPath(), "__complete") || | ||
len(fields) > 1 && fields[1] == "help" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to use Fields
to protect against the case where a user has help
in some path passed to the other commands. If that isn't necessary, we should be fine using Contains
directly.
cobra
has c.Name()
, but that gets hairy, since the completion commands are actually subcommands, so it initially returns the shell name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should swap the logic around. Instead of PersistentPreRun
we can do a PreRun
on all commands that require root. This can be a fairly simple:
for _, cmd := range []interface{createKeysCmd(), enrollKeysCmd(), .....}{
cmd.Prerun = func(....iforget) {
// if check
}
rootCmd.AddCommand(cmd)
}
Add an variable for the commands or something. Wouldn't this be simpler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has a chance of being much cleaner. I will see if I can implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also abstract away the for loop and have
func WithRoot(cmds ...interface{}) *cobra.Command {
for _, cmd := range cmds {
... something
}
}
func main(){
rootCmd = WithRoot(
createKeysCmd().
enrollKeysCmd(),
....etc
)
}
That said, I was considering removing this check entirely, due to the possibility of adding integration tests. Doing an acess test with the databases and/or keys would allow us to check for adequate permissions without hardcoding the required UID (or getting ugly errors). For testing (unless it's always done inside a chroot environment, which I don't love), we would require command line options for the location of the |
Ping on the idea of moving to some access test instead of checking if uid is 0. |
Yes, a much better idea instead of lazily checking for root. |
Ok, I've kinda turned the PR on its head; feel free to point out any issues you have with my changes, since I'm not 100% sure about them either. I still need to review the changes again, but they look somewhat sane for now. |
I need to pull and test this, but I think this is a better approach then gateing the command at the top-level. Allows for more fine-grained permission handling as well! Thanks for the work :) |
Happy to help :) If you don't think the PR will get too bloated, I could start adding stuff for the exit error codes as well. Though that would likely delay the PR a bit. |
Ping? |
database.go
Outdated
} else { | ||
if os.IsPermission(err) { | ||
warning.Printf(rootMsg) | ||
} | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else { | |
if os.IsPermission(err) { | |
warning.Printf(rootMsg) | |
} | |
return nil, err | |
} else if os.IsPermission(err) { | |
warning.Printf(rootMsg) | |
return nil, err | |
} |
Needs to be written like that, else we will always hit the else clause and return which makes you unable to sign. I think the root issue is that we don't really check if err is nil or not. If err is nil, the first if is False
which gives us a return. This code is only handling permission error, but nothing else.
Might be an improvement for another time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If err is nil, the first if is False which gives us a return. This code is only handling permission error, but nothing else.
I believe checking explicitly for err != nil
is best, thanks for pointing this out!
I've dropped my usage of As explained in the commit message, it's still necessary in |
The replies are a bit inconsistent:
Part of the problem here is that |
Oh, well spotted. I will try to organize it better. |
This allows err to be used anywhere as the error variable, instead of having to use "e", for example. This commit also fixes a bug where the PrintGenerateError() calls in CombineFiles() were using "err" as the argument for error, when it should have been "e" - since "err" was the logger and could be used in that way, the compiler didn't complain. Signed-off-by: Érico Rolim <erico.erc@gmail.com>
If ReadFile errors out, the error would only be checked after the function attempts to read the buffer into the hasher. This commit fixes that, checking the error as soon as possible. Signed-off-by: Érico Rolim <erico.erc@gmail.com>
- Introduces dependency on sys/unix for unix.Access. This is necessary only in keys.go, since we run 'sbsign' as a command and can't check if it failed due to permissions. - Allows removing special casing in main.go for commands that don't require root permissions. - ReadFileDatabase() can now return errors due to the multiple ways in which it can fail; it also warns the user about possibly requiring root. - ReadFileDatabase() was using the global DBPath instead of its dbpath parameter in multiple places. This has been fixed. - VerifyESP() can now run without root. - SignFile() checks if it can read the DB key before running sbsign. Signed-off-by: Érico Rolim <erico.erc@gmail.com>
This function will try to read a file into a byte buffer, and, if the file doesn't exist, create its containing directory and the file itself. If any of those actions fail due to permissions, the function will print a warning about running the tool as root. Reading from the file and bundle databases works like this, so the error checking should be implemented in a single place. Also, use the new function in ReadFileDatabase(). Signed-off-by: Érico Rolim <erico.erc@gmail.com>
Using the function also removed code that had hardcoded globals for the location of some files instead of using the dbpath parameter. Add error checking around the function where appropriate. Also fail early when creating a new bundle if it isn't possible to access the bundle database. Signed-off-by: Érico Rolim <erico.erc@gmail.com>
Ok, the PR has increased a bit. I haven't checked extensively if the UI got more consistent, but at least the code should have. It's quite a few changes, but I believe you should be able to follow them logically by looking at the commit history; I tried to explain each change as well as possible. |
I'm not sure I need to check the array length, because I don't think it enters the function if there isn't any command at all. But I didn't want to risk it.
Could be changed to
strings.Fields(c.CommandPath())[1]
directly.